fix(embed): join startup workers and preserve service failures - #28656
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
89cc564 to
490e0ff
Compare
490e0ff to
12e7513
Compare
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Reviewed 12e7513 against main ee0c2bf. No concrete new blocker found; COMMENT only for this self-authored PR.
Each launched CN writes a distinct preallocated error slot. WaitGroup completion precedes reading those slots, returning to Start/StartNewCNService, and rollback. Thus heterogeneous error types no longer go through atomic.Value, all returned failures remain in the error tree in configuration order, and a synchronous infrastructure failure stops further scheduling without abandoning earlier CN workers. The result array is invocation-local, so incremental starts and subsequent attempts cannot inherit old errors. Normal startup retains CN concurrency with bounded O(service count) additional storage.
I traced the caller cleanup boundaries: initial startup rolls back only after the join; incremental startup retains its existing rollback/pending-cleanup ownership. The service object is transferred to the operator before its Start call, and failed Close retains ownership. The change does not introduce a new callback/cluster-lock dependency in the normal service startup path.
Important limits: join is not cancellation or a global startup deadline. Existing readiness waits have their own bounds, but service construction/Start and a deliberately blocking startFn are not universally bounded by this helper. CN-only startup already waited for all workers; the new wait after synchronous infrastructure failure intentionally trades early return for safe ownership. A worker panic is still a process panic, not a collected service error. These are not newly solved guarantees.
One non-blocking API clarification: errors.Is still finds the original cause, and errors.As finds distinct concrete cause types such as os.PathError as tested. However, because the attribution wrapper is itself a *moerr.Error and precedes the cause in errors.Join, errors.As into *moerr.Error selects the new internal-error wrapper before an underlying typed MO error. Do not describe this as preserving every errors.As selection/error code unchanged; I found no affected startup consumer requiring the old selection.
The deterministic tests cover heterogeneous causes, ordering/identity, an incremental success after failure, and joining an earlier CN before returning a later LOG failure while not starting TN. Existing rollback tests remain. Optional latch deadlines would make a scheduling regression fail faster than the package timeout; they are not evidence of a current false pass.
Evidence/limits: complete two-file diff, PR discussion/history, exact-head initial/incremental startup, operator ownership/readiness and cleanup paths inspected; head/base rechecked. Reported full-package/race repetitions/vet/lint are author evidence, not independent reruns. No new service startup, fault run or CI wait.
…xorigin#28656) ## What type of PR is this? - [x] BUG - [x] Test and CI ## Which issue(s) this PR fixes: Related: matrixorigin#28419 ## What this PR does / why we need it: Embedded cluster startup can launch CN workers concurrently, but the previous implementation stored heterogeneous startup errors in one `atomic.Value`. Different concrete error types can panic while being stored, and a synchronous LOG/TN failure could return while an already-launched CN worker was still starting. That lets rollback overlap startup and hides the original failure. The startup path now owns one error slot per service, joins every launched CN worker before returning, keeps errors in service configuration order, preserves `errors.Is`/`errors.As`, and stops scheduling services after a synchronous infrastructure failure. The fix applies to initial and incremental startup. The regression tests use the existing `startFn` seam, so they do not start real services or add sleeps. The error wrapper uses MatrixOne's typed internal error to satisfy the repository SCA contract. The concurrent-error oracle has been updated to assert the resulting diagnostic while retaining the cause and ordering checks. ## Validation - Focused startup selection is non-empty and passes in normal mode. - `TestDoStartLockedConcurrentErrors`: `-race -count=100`, passed (1.192s). - `TestDoStartLockedJoinsCNBeforeReturningInfrastructureFailure`: `-race -count=100`, passed (1.194s). - Full `pkg/embed` normal: passed (49.268s). - Full `pkg/embed` race: passed (55.684s). - `go vet ./pkg/embed/...`: passed. - Incremental `golangci-lint --new-from-rev origin/main ./pkg/embed/...`: 0 issues. - `git diff --check`: passed. The tests were run from the rebased candidate worktree at main `ee0c2bf563bf27ff2c80891bb57a1de5886bf90b` with a freshly built CPU CGo/native dependency set. The first vet attempt hit a full `/tmp` before analysis; it was rerun with disk-backed temporary storage and passed.
…xorigin#28656) ## What type of PR is this? - [x] BUG - [x] Test and CI ## Which issue(s) this PR fixes: Related: matrixorigin#28419 ## What this PR does / why we need it: Embedded cluster startup can launch CN workers concurrently, but the previous implementation stored heterogeneous startup errors in one `atomic.Value`. Different concrete error types can panic while being stored, and a synchronous LOG/TN failure could return while an already-launched CN worker was still starting. That lets rollback overlap startup and hides the original failure. The startup path now owns one error slot per service, joins every launched CN worker before returning, keeps errors in service configuration order, preserves `errors.Is`/`errors.As`, and stops scheduling services after a synchronous infrastructure failure. The fix applies to initial and incremental startup. The regression tests use the existing `startFn` seam, so they do not start real services or add sleeps. The error wrapper uses MatrixOne's typed internal error to satisfy the repository SCA contract. The concurrent-error oracle has been updated to assert the resulting diagnostic while retaining the cause and ordering checks. ## Validation - Focused startup selection is non-empty and passes in normal mode. - `TestDoStartLockedConcurrentErrors`: `-race -count=100`, passed (1.192s). - `TestDoStartLockedJoinsCNBeforeReturningInfrastructureFailure`: `-race -count=100`, passed (1.194s). - Full `pkg/embed` normal: passed (49.268s). - Full `pkg/embed` race: passed (55.684s). - `go vet ./pkg/embed/...`: passed. - Incremental `golangci-lint --new-from-rev origin/main ./pkg/embed/...`: 0 issues. - `git diff --check`: passed. The tests were run from the rebased candidate worktree at main `ee0c2bf563bf27ff2c80891bb57a1de5886bf90b` with a freshly built CPU CGo/native dependency set. The first vet attempt hit a full `/tmp` before analysis; it was rerun with disk-backed temporary storage and passed.
…xorigin#28656) ## What type of PR is this? - [x] BUG - [x] Test and CI ## Which issue(s) this PR fixes: Related: matrixorigin#28419 ## What this PR does / why we need it: Embedded cluster startup can launch CN workers concurrently, but the previous implementation stored heterogeneous startup errors in one `atomic.Value`. Different concrete error types can panic while being stored, and a synchronous LOG/TN failure could return while an already-launched CN worker was still starting. That lets rollback overlap startup and hides the original failure. The startup path now owns one error slot per service, joins every launched CN worker before returning, keeps errors in service configuration order, preserves `errors.Is`/`errors.As`, and stops scheduling services after a synchronous infrastructure failure. The fix applies to initial and incremental startup. The regression tests use the existing `startFn` seam, so they do not start real services or add sleeps. The error wrapper uses MatrixOne's typed internal error to satisfy the repository SCA contract. The concurrent-error oracle has been updated to assert the resulting diagnostic while retaining the cause and ordering checks. ## Validation - Focused startup selection is non-empty and passes in normal mode. - `TestDoStartLockedConcurrentErrors`: `-race -count=100`, passed (1.192s). - `TestDoStartLockedJoinsCNBeforeReturningInfrastructureFailure`: `-race -count=100`, passed (1.194s). - Full `pkg/embed` normal: passed (49.268s). - Full `pkg/embed` race: passed (55.684s). - `go vet ./pkg/embed/...`: passed. - Incremental `golangci-lint --new-from-rev origin/main ./pkg/embed/...`: 0 issues. - `git diff --check`: passed. The tests were run from the rebased candidate worktree at main `ee0c2bf563bf27ff2c80891bb57a1de5886bf90b` with a freshly built CPU CGo/native dependency set. The first vet attempt hit a full `/tmp` before analysis; it was rerun with disk-backed temporary storage and passed.
This branch was successfully deployed
2 participants
What type of PR is this?
Which issue(s) this PR fixes:
Related: #28419
What this PR does / why we need it:
Embedded cluster startup can launch CN workers concurrently, but the previous implementation stored heterogeneous startup errors in one
atomic.Value. Different concrete error types can panic while being stored, and a synchronous LOG/TN failure could return while an already-launched CN worker was still starting. That lets rollback overlap startup and hides the original failure.The startup path now owns one error slot per service, joins every launched CN worker before returning, keeps errors in service configuration order, preserves
errors.Is/errors.As, and stops scheduling services after a synchronous infrastructure failure. The fix applies to initial and incremental startup. The regression tests use the existingstartFnseam, so they do not start real services or add sleeps.The error wrapper uses MatrixOne's typed internal error to satisfy the repository SCA contract. The concurrent-error oracle has been updated to assert the resulting diagnostic while retaining the cause and ordering checks.
Validation
TestDoStartLockedConcurrentErrors:-race -count=100, passed (1.192s).TestDoStartLockedJoinsCNBeforeReturningInfrastructureFailure:-race -count=100, passed (1.194s).pkg/embednormal: passed (49.268s).pkg/embedrace: passed (55.684s).go vet ./pkg/embed/...: passed.golangci-lint --new-from-rev origin/main ./pkg/embed/...: 0 issues.git diff --check: passed.The tests were run from the rebased candidate worktree at main
ee0c2bf563bf27ff2c80891bb57a1de5886bf90bwith a freshly built CPU CGo/native dependency set. The first vet attempt hit a full/tmpbefore analysis; it was rerun with disk-backed temporary storage and passed.